refactor: remove params from config and introduce app context #1774
Conversation
There was a problem hiding this comment.
I think the global direction of sperating the runtime values into a context object and having a high level app context is good.
Would be great to have a look from @ws4charlie (he's on public holiday today)
|
@lumtis thanks for review, i fixed some comments, left some of bigger questions (such as using pointers and missing docs) to check further this PR was more to get thoughts on overall approach, if you guys think its fine i will fine tune remaining details, let's also wait for @ws4charlie review though |
|
@skosito I see PR comments have been addressed, is it ready to be reviewed? |
i was fixing smoke tests, just want to do a bit more testing, will convert to ready for review in couple of hours |
There was a problem hiding this comment.
This looks good to me. Few suggestions about adding more comments.
@ws4charlie can you have a look today? I think it would be great to merge soon as it will have conflicts with the restricted addresses PR
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1774 +/- ##
========================================
Coverage 38.03% 38.03%
========================================
Files 205 205
Lines 12526 12524 -2
========================================
Hits 4764 4764
+ Misses 7374 7372 -2
Partials 388 388
|
|
@lumtis @kingpinXD I opened up an issue on missing thins you mentioned on pointers and core context unit tests #1807, I can pick this issue up soon so it is addressed. Using separate issue because this is leftover from previous solution and to keep this PR small and short lived. Please let me know if this is ok and not a blocker to merge this PR. |
|
@ws4charlie could you please check PR again when you get a chance, I merged latest develop with your changes and fixed conflicts and addressed your comments also, initially i put masterLogger in appContext, but i see you introduced clientLoggers in your PR, so I decided to keep it out of app context since it is not scope of this PR, and to not change your implementation - please let me know if you think it belongs to app context and we can create issue to return it, or do it in this PR |
* simplify docker compose * fix makefi;e * add new version * type * fix restart client * some fixes * add light upgrade test * fix gov sending * add admin function test command * performance test * fix lint * add pprof port * some fixes in tests * test: emissions unit test (#1767) * ci: run build workflow on develop push for code coverage generation (#1793) * refactor: zetaclient metrics (#1783) * Refactor zetaclient metrics * Fix blame counter init * Add changelog entry * Add unreleased to changelog * Remove blank line * feat: initiated feature of zetaclient-restricted-address (#1790) * initiated feature of zetaclient-banned-address * fix some issues in e2e tests * fix gosec issues * some unit tests, comments and refactor * unified log prints for restricted address detection * move restricted address to Unreleased section * improved compliance log prints * update compliance log print according to CharlieMc * chore(codecov.yml): fix ignore extension (#1814) * chore(codecov.yml): fix ignore extension * changelog * refactor: remove params from config and introduce app context (#1774) * refactor(`crosschain`): move ballot voting logic into `observer` (#1511) * move logic for ballot voting for inbound * fix mocks * goimports * changelog * fix unit tests * fix CI = * x/observer/keeper/vote_outbound.go x/crosschain/types/expected_keepers.go x/crosschain/keeper/msg_server_vote_outbound_tx.go * update mocks * refactor finalized check * make generate * initialize vote inbound tests * add sdk keepers in observer testutil * implement mock option for observer * inbound vote test * initialize outbound tests * goimports * vote outbound tests * update changelog * test with integration tests * add isNew in vote_inbound * add tests back * use tmp context for ballot logic * add new test for finalized ballot * add not finzalized test * remove unused comment * fix bitcoin credentials * change upgrade version * fix performance tests * handler * fix setup handler * add light flag and use it for upgrade tests * fix username * add performance test back * Update cmd/zetae2e/local/ethereum.go * stefan comments * changelog * fix --------- Co-authored-by: Tanmay <[email protected]> Co-authored-by: skosito <[email protected]> Co-authored-by: Charlie Chen <[email protected]>
Description
Main purpose of linked issue is to remove runtime params from config.
Simple split of these params and config resulted in one more argument being passed around from top level (called
ZetaCoreContext). However, since config and these params are still related, it makes sense to have them together in top level struct (calledAppContext).I also added
masterLoggertoAppContextas illustration and proposal that these common application context specific things can be grouped together and passed around and accessible as needed in convenient way.This is opened as draft PR to get some feedback on this solution and then will be converted in proper PR and tested.
Closes: #1608
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.
Still in draft phase.
Checklist: